Skip to content

feat: retain entity relationship evidence in extracted events - #22

Closed
Pigbibi wants to merge 3 commits into
mainfrom
codex/p0-1-extraction-schema
Closed

feat: retain entity relationship evidence in extracted events#22
Pigbibi wants to merge 3 commits into
mainfrom
codex/p0-1-extraction-schema

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add entity_match_type, match_evidence, and relationship_type to normalized event schema
  • classify all aliases and retain the strongest evidence; prioritize direct-beneficiary patterns and trusted issuer-release canonical names
  • downgrade generic aliases to industry_context without changing tracker/event-study scoring
  • add regressions for MSTR, OKLO, PANW, COIN, INTC, alias order, and legacy schema defaults

Scope

  • Extraction/schema only; no tracker or event-study behavior changes.
  • Research-only; no execution, allocation, broker, or production dependency changes.

Validation

  • PYTHONPATH=src python3 -m pytest -q tests/test_source_mention_extract.py tests/test_official_event_import.py (14 passed)
  • PYTHONPATH=src python3 -m pytest -q (24 passed)
  • PYTHONPATH=src python3 -m compileall -q src tests
  • git diff --check

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Codex PR Review

🚫 Merge blocked: 2 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: The current findings do not all remain valid. Finding 1 conflicts with the prior blocking contract and is disproved by the current source-of-truth tests: load_aliases() stores name separately, match_evidence() only scans alias_record.aliases, and tests/test_source_mention_extract.py::test_curation_omission_keeps_canonical_name_as_metadata_only explicitly asserts no event is emitted when name is present but omitted from aliases (PANW,Palo Alto Networks,PANW with text mentioning only Palo Alto Networks). That is direct contract evidence that canonical name must stay metadata-only unless curated into aliases, so suggesting implicit matching would reverse the prior required behavior. Finding 2 remains valid against the cumulative diff: in src/political_event_tracking_research/source_mention_extract.py, match_evidence() assigns relationship = "issuer" whenever source_type == "issuer_release" and the matched alias equals alias_record.name; is_canonical_name checks only alias text equality, and there is no verification against any issuer-identity field in RawSourceItem (which has only item_id, published_at, source_type, source_url, author, text) or elsewhere. Because extract_source_records() applies this to every alias record, an issuer release from company A that mentions company B by canonical name will still classify B as issuer. No current test or schema proves that behavior false or fixed.

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in src/political_event_tracking_research/source_mention_extract.py

load_aliases() stores the new name field, but match_evidence() only searches alias_record.aliases. As a result, a company's canonical name is ignored unless it is redundantly copied into the aliases column, so issuer-release texts that mention the real company name can produce no event at all. This is a wrong-results regression for any alias dataset that keeps name and aliases separate. (line 81)

Suggestion: Treat name as an implicit alias during matching, or have match_evidence() test alias_record.name directly in addition to alias_record.aliases.

2. 🟠 [HIGH] Logic in src/political_event_tracking_research/source_mention_extract.py

The new issuer classification is assigned whenever source_type == "issuer_release" and the matched alias equals the symbol's canonical name. There is no check that the release was actually issued by that company, so a press release from company A that mentions company B will incorrectly classify B as issuer rather than a third-party mention. (line 130)

Suggestion: Only assign issuer when the raw item carries verified issuer identity and it matches the alias record; otherwise keep canonical-name matches in issuer releases as unverified until the issuer can be validated.


Review by Codex PR Review bot • PR

Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1d21f8b7b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +141 to +142
if direct_pattern.search(normalized_text):
relationship = "direct_beneficiary"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Prevent generic phrases from becoming direct beneficiaries

When a denylisted alias appears after a direct-beneficiary cue, this branch runs before the generic-alias check below, so text such as funding for cybersecurity is emitted as direct_beneficiary for PANW/CRWD rather than industry_context. That turns sector-level funding language into company-level evidence and can corrupt the relationship fields the commit is adding; the direct pattern should not promote aliases that are only generic context terms.

Useful? React with 👍 / 👎.

normalized = normalize_match_text(alias).strip()
if normalized.upper() == alias_record.symbol.upper():
return False
return normalized.casefold() in GENERIC_ALIAS_DENYLIST

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize configured generic alias variants

This exact membership check misses generic aliases already present in config/core_us_equity_aliases.csv, for example energy related infrastructure alongside the denylisted hyphenated form on config lines 9, 24, 40, and 41. A government-policy item containing the unhyphenated phrase therefore falls through to issuer evidence instead of industry_context; normalize equivalent hyphen/space variants or include the configured variants in the generic classification.

Useful? React with 👍 / 👎.

Comment on lines +200 to +204
for alias_record in aliases:
evidence = match_evidence(item.text, alias_record, item.source_type)
if evidence is None:
continue
entity_match_type, matched_text = evidence

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Deduplicate repeated symbols after evidence matching

When an operator-provided aliases CSV has more than one row for the same symbol, this per-alias-record loop appends one event for each row; unlike the previous match_symbols path, nothing deduplicates by symbol before writing. That produces duplicate event_id rows for one source item, which downstream tracker/event-study code counts as separate events and can inflate scores or returns unless the strongest evidence is collapsed per (item_id, symbol).

Useful? React with 👍 / 👎.

Comment on lines +136 to +137
rf"(?:awarded to|contract with|funding for|benefit(?:s|ed)? from)\s+"
rf"(?:the\s+)?{re.escape(normalized_alias)}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Anchor direct-beneficiary aliases at word boundaries

Because this direct-beneficiary regex inserts the escaped alias without the same trailing boundary used by alias_pattern, an item that mentions a company elsewhere can be upgraded by a prefix match in an unrelated beneficiary, e.g. Ford earnings were discussed. Funding for Fordham University... returns direct_beneficiary for Ford. Reusing the alias boundary here avoids turning longer names that merely start with a ticker/company alias into direct company evidence.

Useful? React with 👍 / 👎.

Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21f29cebfb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return None
relationship, evidence, _ = max(
matches,
key=lambda match: (RELATIONSHIP_PRIORITY[match[0]], match[2], len(match[1]), match[1].casefold()),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Prefer explicit aliases over generic context

When one item contains both a real company alias and a denylisted context alias for the same symbol, this ordering causes the generic match to win because industry_context outranks unverified; for example, PANW aliases include both Palo Alto Networks and cybersecurity, so a government item like Palo Alto Networks cybersecurity guidance is emitted as industry_context with match_evidence=cybersecurity. That loses the explicit company-name evidence in the new relationship fields, so generic aliases should only win when no non-generic company/ticker alias matched.

Useful? React with 👍 / 👎.

if not alias_pattern(alias).search(normalized_text):
continue
direct_pattern = re.compile(
rf"(?:awarded to|contract with|funding for|benefit(?:s|ed)? from)\s+"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove reversed benefits-from cue

When a sentence says another party benefits from a company, this cue treats the company after benefits from as the direct beneficiary. For example, a government item like Domestic suppliers benefit from Intel investment marks INTC as direct_beneficiary, even though the beneficiaries are the suppliers and Intel is just source/context, which overstates company-level support in the new relationship fields.

Useful? React with 👍 / 👎.

Comment on lines +141 to +142
elif source_type == "issuer_release" and is_canonical_name:
relationship = "issuer"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not trust third-party names in issuer releases

When any issuer_release item mentions another covered company by its canonical name, this branch promotes that other company to issuer evidence even though source_items.csv has no field identifying which issuer owns the release. For example, an NVIDIA issuer release saying it partnered with Palo Alto Networks would emit PANW as issuer evidence instead of a third-party mention; only promote issuer evidence when the release can be tied to the same symbol.

Useful? React with 👍 / 👎.

@Pigbibi

Pigbibi commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Superseded after the single allowed remediation. Re-slicing from latest main into a schema/serialization-only contract; matcher behavior will follow in a separate PR. No further changes will be made on this PR.

@Pigbibi Pigbibi closed this Jul 13, 2026
@Pigbibi
Pigbibi deleted the codex/p0-1-extraction-schema branch July 13, 2026 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant